Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line2 world units raycaster #23358

Merged
merged 19 commits into from
Mar 9, 2022

Conversation

bergden-resonai
Copy link
Contributor

@bergden-resonai bergden-resonai commented Jan 27, 2022

Fixed #22611.

Description

  • Adding raytracing (and thresholds) to the lines_fat example
  • Respecting worldUnits in ray tracing for LineSegments2

This contribution is funded by Resonai.

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for the addition.

The required raycasting logic between world-unit lines and screen-space thickness lines should be sufficiently different that I think the logic would be easier to follow if there were two separate loops for each case. There's also a lot of unnecessary computation happening with world unit lines, as well:

if ( worldUnits ) {

    // for loop finding world unit intersections

} else {

    // for loop finding screen space intersections

}

And a couple other thoughts:

  • I think we should make the intersection spheres in the example a lot smaller so it's more clear where the intersection points are being reported.
  • It might be best to treat the world-unit line intersections as capsules similar to the way they're being rendered? As it is it looks like the "hit" point is the point of closest approach, instead, meaning the hit point is not necessarily on the surface. This is happening with screenspace lines, too, though it might be more noticeable with world unit lines. Not sure if that has to happen in this PR, though.

Live link to the updated example:

https://raw.githack.com/bergden-resonai/three.js/line2-world-units-raycaster/examples/webgl_lines_fat.html

cc @WestLangley not sure if you have other thoughts

examples/jsm/lines/LineSegments2.js Outdated Show resolved Hide resolved
examples/jsm/lines/LineSegments2.js Outdated Show resolved Hide resolved
examples/jsm/lines/LineSegments2.js Outdated Show resolved Hide resolved
@WestLangley
Copy link
Collaborator

I would create a separate example for raycasting with fat lines.

build/three.cjs Outdated Show resolved Hide resolved
Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example looks great, thanks!

https://raw.githack.com/bergden-resonai/three.js/line2-world-units-raycaster/examples/webgl_lines_fat_raycasting.html

I've added a couple comments on the complexity of the loops and how to address. Can you explain the "isLoopingInside" variable? It doesn't seem like it should be necessary.


intersects.push( {
if ( ! isInside && isLoopingInside ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes to the non world units case have made this harder to follow. The original goal in writing the loop logic was to make it as close as possible to the shader projection logic and simple so it was easy to verify correctness. I think it would be best to avoid modifying the screenspace case as much as possible here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to dev's version - #23358 (comment)

Comment on lines 269 to 284
// skip the segment if it's entirely behind the camera
const isBehindCameraNear = _start4.z > near && _end4.z > near;
if ( isBehindCameraNear ) {

// skip the segment if it's entirely behind the camera
const isBehindCameraNear = _start4.z > near && _end4.z > near;
if ( isBehindCameraNear ) {
continue;

continue;
}

}
// trim the segment if it extends behind camera near
trimSegment( near );

// trim the segment if it extends behind camera near
if ( _start4.z > near ) {
if ( ! isInClipSpace( projectionMatrix, resolution ) ) {

const deltaDist = _start4.z - _end4.z;
const t = ( _start4.z - near ) / deltaDist;
_start4.lerp( _end4, t );
continue;

} else if ( _end4.z > near ) {
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the world units lines there's no reason to ensure the lines are in clip space anymore. The only reason the camera is required otherwise is because it's necessary they are in screen space. I think the simplest approach would be to iterate over the lines without any screenspace or camera logic and checking ray / line segment distances for intersections. Then there's no requirement that the lines be visible in order to be interacted with. Let me know if that sounds good to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand - let's say we have a very long line that's going from behind the camera near to inside the camera frustum
We don't want the parts of the lines before the camera near to be considered, but if we only keep the intersections and try to calculate it at the end we will be missing some information - no?

I agree about the clipSpace, remove the if block for that
Let me know if you meant to remove the trimSegment and isBehindCameraNear as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, out of curiosity - why don't we need to consider far?
The raytracer removes points that are behind the far plane - but sometimes a closer part of the line might fall within the threshold so we would miss intersections there, am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason that world lines have to be handled in screen space. The only reason the screen space lines used the camera is because it's required based on how they're displayed. Picking effectively happens in 2D and given the way they're rendering proper picking is effectively undefined outside of the camera clip space. It's definitely a special case for the screen space line variant.

This isn't the case with the world unit lines, though. They're basically rendered as capsules. Meshes, for example, don't have special handling when they're behind the camera and don't consider the camera at all. This enables rays that are not derived from a camera and mouse coordinate to be used for raycasting (ie vr controller rays outside of view or rays for game mechanics outside of view)

The built in GL line raycasting doesn't use a camera and includes support for a threshold which effectively means they're treated as capsules. I think that's more along the lines of what I imagine for world unit raycasting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean the "computeStartEnd", "trimSegment", and "isInClipSpace" functions do not need to be shared and don't need to be pulled out into separate functions. I just want to keep the changes to the original logic minimal.

Copy link
Contributor Author

@bergden-resonai bergden-resonai Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I forgot to update - I changed the main camera's near to 10 to test behavior and luckily committed it by accident so it can still be tested:

Screen.Recording.2022-02-19.at.19.33.23.mov

The lines are rendered truncated by the near plane and the intersection still happens (denoted by the cursor and the little canvas on the bottom left which still has near set to 1) - which I'm pretty sure is a bug in my last commits
I just want to confirm - this is the desired behavior? if so I'll remove the calls to trimSegment/isBehindCamera instead of debugging

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a function of the math involved. The lines are rendered as capsules and have thickness -- you're not seeing back faces of the capsule but technically the near plane (and therefore the ray start) is within the capsule and registers as a "hit" within the given threshold.

@bergden-resonai
Copy link
Contributor Author

Made a lot of changes, hopefully without new bugs:

  1. Split raycasting for world units and non world units (how do we want to call this?)
  2. Removed dashed/gl.line from the example, added visualizations for the thresholds to help debug and explain it, made it easy to switch from segments to lines, the colors of the points change to increase contrast with the intersected segment
  3. Minor code readability improvements
  4. Remove isLoopingInside which to best of my knowledge (I need to sync with @ariel-resonai on that) tried to solve the case where if segment1 and segment2 were overlapping in screen space after factoring the threshold, but the cursor was obviously over segment1 the intersection returned would prefer segment2 since it's closer (where Ariel wanted to minimize the distance between the point and pointOnLine)

With inLoopingInside:
https://user-images.githubusercontent.com/37291459/154805071-8f47bc40-f0ca-4803-b891-d0059387ad67.mov

Without:
https://user-images.githubusercontent.com/37291459/154805089-50b27b6e-c3a9-40cc-8da2-2e8258894e89.mov

Open questions:
A. I'm not sure why there's a discrepancy between the threshold visualization and the intersections found in non-worldUnits mode
image
B. This

@gkjohnson
Copy link
Collaborator

Split raycasting for world units and non world units (how do we want to call this?)

I think world units and screen space is good.

Remove isLoopingInside which to best of my knowledge (I need to sync with @ariel-resonai on that) tried to solve the case where if segment1 and segment2 were overlapping in screen space after factoring the threshold, but the cursor was obviously over segment1 the intersection returned would prefer segment2 since it's closer (where Ariel wanted to minimize the distance between the point and pointOnLine)

I think if we want to improve screen space line behavior it should be done in a separate PR and this should focus on just adding world space line raycasting. The number of changes has made it harder to follow exactly what's been changed so far. The screen space line logic was a fairly complicated piece of logic to add so I'm less keen on making any changers to it here.

That aside there are ambiguities / redundancies at points where segments overlap which causes the popping but I think that's just the nature of this kind of raycasting. I believe it happens with the GL line variant, as well.

A. I'm not sure why there's a discrepancy between the threshold visualization and the intersections found in non-worldUnits mode

Can we confirm that this happens with the original code on master? I just want to make sure it's not a newly introduced issue. If it's on master I think you need to worry about it here and it can be addressed in a different PR.

@bergden-resonai
Copy link
Contributor Author

Split raycasting for world units and non world units (how do we want to call this?)

I think world units and screen space is good.

Remove isLoopingInside which to best of my knowledge (I need to sync with @ariel-resonai on that) tried to solve the case where if segment1 and segment2 were overlapping in screen space after factoring the threshold, but the cursor was obviously over segment1 the intersection returned would prefer segment2 since it's closer (where Ariel wanted to minimize the distance between the point and pointOnLine)

I think if we want to improve screen space line behavior it should be done in a separate PR and this should focus on just adding world space line raycasting. The number of changes has made it harder to follow exactly what's been changed so far. The screen space line logic was a fairly complicated piece of logic to add so I'm less keen on making any changers to it here.

That aside there are ambiguities / redundancies at points where segments overlap which causes the popping but I think that's just the nature of this kind of raycasting. I believe it happens with the GL line variant, as well.

A. I'm not sure why there's a discrepancy between the threshold visualization and the intersections found in non-worldUnits mode

Can we confirm that this happens with the original code on master? I just want to make sure it's not a newly introduced issue. If it's on master I think you need to worry about it here and it can be addressed in a different PR.

I took the current example and used it with the dev version of LineSegments2.js and saw the same behavior - I'll open an issue after this PR
Ack on rest of comments here

@bergden-resonai
Copy link
Contributor Author

Hopefully this time everything's sorted out

I couldn't make the diff work for the current way the code is organized, but I reverted the raycasting function to mrdoob/dev version. I kept the split in the functions because I the if/else was getting too long (thus the alleged big diff you see in the PR). There are minor changes in variable/global variable names, and the functionality regarding sphere/box is common so I kept it outside the type-specific functions

Screen.Recording.2022-02-19.at.20.13.48.mov

@WestLangley
Copy link
Collaborator

The inset in the original example was added to study the impact of canvas size and the user-set resolution parameter.

I would not include the inset viewport in the raycasting example -- unless there is a compelling reason to do so.

@bergden-resonai
Copy link
Contributor Author

The inset in the original example was added to study the impact of canvas size and the user-set resolution parameter.

I would not include the inset viewport in the raycasting example -- unless there is a compelling reason to do so.

This example also showcases how the resolution affects the rendered lines relative size
And also this really helped me catch a few bugs when the cameras didn't match in their position/near/far (I reverted these changes since it's easy but reviving the smaller canvas is harder if I remove it)
I don't know if this is compelling enough :)

@WestLangley
Copy link
Collaborator

I don't know if this is compelling enough :)

@bergden-resonai Thank you for your contribution. I'll defer to whatever @gkjohnson recommends. :-)

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks again.

I'll defer to whatever @gkjohnson recommends. :-)

I don't have any strong feelings on this. It can always be adjusted later.

I don't

@WestLangley WestLangley added this to the r139 milestone Feb 27, 2022
@mrdoob mrdoob merged commit aa01ab3 into mrdoob:dev Mar 9, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2022

Thanks!

donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* Try brute force...

* Add example and clean-up code

* Remove comment

* Applying PR comments, bug fix, creating new example for raycasting

* adding new example to the tags

* Add example icon and undo build/ changes

* Revert build files

* Checkout build/ from updated upstream

* WIP refactor to split logic

* Bug fixes and cleanup

* Fix screenshot

* PR notes

* Cleaning up

* More cleanup

* Bug fixes

Co-authored-by: ariel-resonai <[email protected]>
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Try brute force...

* Add example and clean-up code

* Remove comment

* Applying PR comments, bug fix, creating new example for raycasting

* adding new example to the tags

* Add example icon and undo build/ changes

* Revert build files

* Checkout build/ from updated upstream

* WIP refactor to split logic

* Bug fixes and cleanup

* Fix screenshot

* PR notes

* Cleaning up

* More cleanup

* Bug fixes

Co-authored-by: ariel-resonai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line2's raycaster threshold not compatable with LineMaterial's 'worldUnits'
5 participants